feat(diff): register kb.diff at all four kb.* surface sites#344
Merged
Conversation
github-actions
Bot
added
docs
📝 WalkthroughWalkthrough
Changeskb.diff optional new_id and MCP/JSONL parity
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as cli.py
participant MCP as server.py
participant JSONL as jsonl_server.py
participant Diff as diff.py
participant Store as KBStore
CLI->>Diff: diff_artifacts(store, old_id, new_id?)
MCP->>Diff: diff_artifacts(_store(), old_id, new_id?)
JSONL->>Diff: diff_artifacts(_store(), old_id, new_id?)
Diff->>Store: load old artifact
Diff->>Store: read superseded_by when new_id omitted
Diff-->>CLI: ArtifactDiff / DiffError
Diff-->>MCP: ArtifactDiff
Diff-->>JSONL: ArtifactDiff
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/vouch/diff.py (1)
86-96: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReuse the already-fetched claim to avoid a redundant read.
When
new_idis omitted,old_id's claim is fetched here to readsuperseded_by, then fetched again at line 106 to buildold. Sincekb.diffis now also exposed via MCP/JSONL (potentially higher call volume than the CLI), reusing the object avoids an unnecessary disk read + YAML parse per call.♻️ Proposed fix
if new_id is None: if old_kind != "claim": raise DiffError( f"{old_id} is a {old_kind}; pages have no successor pointer, " "pass new_id explicitly" ) old_claim = store.get_claim(old_id) if not old_claim.superseded_by: raise DiffError(f"{old_id} has not been superseded; pass new_id explicitly") new_id = old_claim.superseded_by + else: + old_claim = None new_kind = _kind_of(store, new_id) if new_kind is None: raise DiffError(f"unknown artifact: {new_id}") if old_kind != new_kind: raise DiffError(f"cannot diff {old_kind} against {new_kind}") old: Claim | Page new: Claim | Page if old_kind == "claim": - old, new = store.get_claim(old_id), store.get_claim(new_id) + old, new = old_claim or store.get_claim(old_id), store.get_claim(new_id) fields, text_field = _CLAIM_FIELDS, _CLAIM_TEXT🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/diff.py` around lines 86 - 96, The fallback path in diff logic is doing a redundant claim read when new_id is omitted. Update the diff-building flow around the existing store.get_claim call in the kb.diff path so the fetched old_claim is reused later instead of fetching the same claim again when constructing old, while keeping the superseded_by check and DiffError behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/vouch/diff.py`:
- Around line 86-96: The fallback path in diff logic is doing a redundant claim
read when new_id is omitted. Update the diff-building flow around the existing
store.get_claim call in the kb.diff path so the fetched old_claim is reused
later instead of fetching the same claim again when constructing old, while
keeping the superseded_by check and DiffError behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cdc8645d-68d0-463b-9f75-745ef1b4e124
📒 Files selected for processing (8)
CHANGELOG.mddocs/superpowers/specs/2026-05-25-vouch-diff-design.mdsrc/vouch/capabilities.pysrc/vouch/cli.pysrc/vouch/diff.pysrc/vouch/jsonl_server.pysrc/vouch/server.pytests/test_diff.py
vouch diff shipped CLI-only in 0.1.0, an explicit non-goal at the time
("MCP/JSONL parity ... kb.* surface unchanged"). that leaves it the only
read method skipping the four-site registration convention documented in
CLAUDE.md, so agents talking MCP/JSONL have no way to fetch a revision
diff. adds the kb_diff MCP tool, the kb.diff JSONL handler, and the
capabilities.METHODS entry, next to the other by-id read tools
(kb_read_claim/kb_read_page) with the same unrestricted-read posture.
also makes new_id optional for a superseded claim: diff_artifacts and
the CLI both resolve it from superseded_by when omitted, erroring
clearly when there's no successor (pages still require an explicit
new_id — they have no successor pointer). closes vouchdev#327.
jsdevninja
force-pushed
the
feat/327-kb-diff-parity
branch
from
9b1e733 to
f2b554c
Compare
Collaborator
|
I'm considering to deprecate code diff comparison feature as it takes much effort to run LLMs. Or potentially, we could add it in v2.0.0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
summary
closes #327.
vouch diff <id-old> <id-new>shipped CLI-only in 0.1.0(#86) — an explicit non-goal at the time ("MCP/JSONL parity ... kb.*
surface unchanged"). that left
kb.diffas the only read methodskipping the four-site registration convention (
CLAUDE.md§"When youadd a new kb.* method"), so agents talking MCP or JSONL had no way to
fetch a revision diff.
kb.diffat the remaining three sites:kb_diffMCP toolin
server.py,_h_diff/kb.diffinjsonl_server.py, andcapabilities.METHODS— placed next tokb_read_claim/kb_read_pagewith the same unrestricted-read posture (no
ViewerContextscopefiltering, matching the other by-id reads).
new_idis now optional for a claim:diff_artifactsand the CLIboth resolve it from
superseded_bywhen omitted, erroring clearlywhen there's no successor. pages still require an explicit
new_id(no successor pointer on the
Pagemodel).(
docs/superpowers/specs/2026-05-25-vouch-diff-design.md) to recordthe superseded "CLI-only" decision and the new surface.
src/vouch/diff.py;storage.pyis untouched.
read-only throughout — no proposal emitted, no artifact or audit event
written (asserted directly in
tests/test_diff.py).test plan
pytest tests/test_diff.py tests/test_capabilities.py tests/test_provenance.py -q— 66 passed (20 intest_diff.py,including the new
kb.diffRPC-surface, omitted-new_id, andread-only tests)
mypy src— clean (two pre-existing, unrelatedsandbox.pyerrors on this Windows checkout, confirmed present on
maintoo)ruff check src tests— cleankb.diffregistered at all four sites;test_capabilities(
capabilities.METHODS == HANDLERS.keys()) stays greenSummary by CodeRabbit
New Features
vouch diffsupport across CLI and API surfaces via a newkb.difftool/handler (MCP and JSONL).new_id) is now optional for superseded claims, enabling automatic resolution.Bug Fixes
new_idnow fail with clear, explicit errors.Documentation
vouch diffdesign and changelog to reflect MCP/JSONL parity and the revised resolution rules.